Skip to content

A random-random test for time-series data #132556

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 30 commits into from
Aug 19, 2025
Merged

A random-random test for time-series data #132556

merged 30 commits into from
Aug 19, 2025

Conversation

pabloem
Copy link
Contributor

@pabloem pabloem commented Aug 7, 2025 •
edited
Loading

Follow up items after this PR:

  • Test rate function and counters in general
  • Randomize window size
  • Increase test size
  • Hit some more corner cases (e.g. zero out some parameters)
  • Add a 'fixed schema' test case (as opposed to the current dynamic one)

@pabloem pabloem marked this pull request as ready for review August 11, 2025 18:06
@pabloem pabloem changed the title [wip][do not review] A random-random test for time-series data A random-random test for time-series data Aug 11, 2025
@elasticsearchmachine elasticsearchmachine added the needs:triage Requires assignment of a team area label label Aug 11, 2025
@pabloem pabloem added >test Issues or PRs that are addressing/adding tests :StorageEngine/TSDB You know, for Metrics :StorageEngine/ES|QL Timeseries / metrics / logsdb capabilities in ES|QL and removed needs:triage Requires assignment of a team area label labels Aug 11, 2025
@elasticsearchmachine
Copy link
Collaborator

Pinging @elastic/es-storage-engine (Team:StorageEngine)

not-napoleon
not-napoleon reviewed Aug 11, 2025
View reviewed changes
Copy link
Member

@not-napoleon not-napoleon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd like to get some confirmation from @kkrik-es that this is doing what he wants, but I think it's pretty good. I left some feedback, none of which is critical but I'd like to get it addressed.

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private List<XContentBuilder> documents = null;
private DataGenerationHelper dataGenerationHelper;

private static final class DataGenerationHelper {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if this should be a top level class. Seems like we'll want to build multiple test classes using this framework.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this class to its own file! TY.

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private static Object randomDimensionValue(String dimensionName) {
// We use dimensionName to determine the type of the value.
var isNumeric = dimensionName.hashCode() % 5 == 0;
if (isNumeric) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about IP dimensions?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added 20% of dimensions as IP-like.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as follow up ill add dynamic mapping to parse as ip. thoughts?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not rely on the existing one:
https://github.com/elastic/elasticsearch/blob/main/test/framework/src/main/java/org/elasticsearch/datageneration/fields/leaf/IpFieldDataGenerator.java

pabloem
pabloem commented Aug 12, 2025
View reviewed changes
Copy link
Contributor Author

@pabloem pabloem left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TY @not-napoleon - ptal!

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private List<XContentBuilder> documents = null;
private DataGenerationHelper dataGenerationHelper;

private static final class DataGenerationHelper {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've moved this class to its own file! TY.

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private static Object randomDimensionValue(String dimensionName) {
// We use dimensionName to determine the type of the value.
var isNumeric = dimensionName.hashCode() % 5 == 0;
if (isNumeric) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added 20% of dimensions as IP-like.

...in/esql/src/internalClusterTest/java/org/elasticsearch/xpack/esql/action/GenerativeTSIT.java Outdated
private static Object randomDimensionValue(String dimensionName) {
// We use dimensionName to determine the type of the value.
var isNumeric = dimensionName.hashCode() % 5 == 0;
if (isNumeric) {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

as follow up ill add dynamic mapping to parse as ip. thoughts?

kkrik-es
kkrik-es reviewed Aug 12, 2025
View reviewed changes
Copy link
Contributor

@kkrik-es kkrik-es left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Pablo, this ia a good step. It's nice that you tried to include the pass-through field on the first take, though that complicates things somewhat. I'd start with statically defined dimension and metric fields to get the validation logic in place first, then introduce dynamic fields on top of that.

Let's try to refactor the logic slightly so that it can be further extended in follow-up PRs.

kkrik-es
kkrik-es approved these changes Aug 13, 2025
View reviewed changes
Copy link
Contributor

@kkrik-es kkrik-es left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for addressing the comments. Let's keep iterating.

@pabloem pabloem enabled auto-merge (squash) August 15, 2025 04:08
@pabloem pabloem merged commit 84ba583 into elastic:main Aug 19, 2025
34 checks passed
@romseygeek
Copy link
Contributor

The changes here to FieldType#tryParse() are causing test failures:

./gradlew ":x-pack:plugin:logsdb:javaRestTest" --tests "org.elasticsearch.xpack.logsdb.qa.LogsDbVersusReindexedLogsDbChallengeRestIT.testRandomQueries" -Dtests.seed=F4B510DA34A404A2 -Dtests.locale=ko-KP -Dtests.timezone=EST5EDT -Druntime.java=24

LogsDbVersusReindexedLogsDbChallengeRestIT > testRandomQueries FAILED
    java.lang.IllegalArgumentException: Unknown field type: geo_shape
        at __randomizedtesting.SeedInfo.seed([F4B510DA34A404A2:AA9EA0360C7E293C]:0)
        at org.elasticsearch.datageneration.FieldType.tryParse(FieldType.java:111)
        at org.elasticsearch.datageneration.queries.LeafQueryGenerator.buildForType(LeafQueryGenerator.java:31)

Specifically, the LeafQueryGenerator expects to get null back from unrecognized field types, instead of an Exception.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@not-napoleon not-napoleon not-napoleon left review comments

@kkrik-es kkrik-es kkrik-es approved these changes

Assignees
No one assigned
Labels
:StorageEngine/ES|QL Timeseries / metrics / logsdb capabilities in ES|QL :StorageEngine/TSDB You know, for Metrics Team:StorageEngine >test Issues or PRs that are addressing/adding tests v9.2.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@pabloem @elasticsearchmachine @romseygeek @not-napoleon @kkrik-es